Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

added fallback getting displayInfo when displayInfo is null #3416

Closed
wants to merge 1 commit into from

Conversation

issess
Copy link
Contributor

@issess issess commented Aug 2, 2022

Sometimes, VirtualDisplay DisplayInfo object returns null, So, I fix getting displayInfo from "dumpsys display".

@rom1v
Copy link
Collaborator

rom1v commented Aug 2, 2022

Thank you.

Sometimes, VirtualDisplay DisplayInfo object returns null

Do you know why?

Are there another getDisplayInfo method with different parameter types on your device (if you list all the methods available on manager.getClass())?

I would prefer that over parsing dumpsys display if possible. Otherwise, using it as a fallback looks good to me in principle.

@issess
Copy link
Contributor Author

issess commented Aug 3, 2022

@rom1v Thank you for quick response.

Accessing VirtualDisplay restriced by UID or FLAG_PRIVATE, So It can return null object.

getDisplayInfoInternal(int displayId, int callingUid)
https://cs.android.com/android/platform/superproject/+/master:frameworks/base/services/core/java/com/android/server/display/DisplayManagerService.java;drc=f6247e3d3067fea526382ae32d83f6c650449589;l=633

hasAccess(int uid, int flags, int ownerUid, int displayId)
https://cs.android.com/android/platform/superproject/+/master:frameworks/base/core/java/android/view/Display.java;drc=52be024bc503a12de9baad55140d9a5385e818a3;l=1537

isUidPresentOnDisplayInternal(int uid, int displayId)
https://cs.android.com/android/platform/superproject/+/master:frameworks/base/services/core/java/com/android/server/display/DisplayManagerService.java;drc=f6247e3d3067fea526382ae32d83f6c650449589;l=1362

I already tried list all methods in IDisplayManager, there are no way to get virtualdisplay displayInfo. I use dumpsys display , And then It works!

@rom1v
Copy link
Collaborator

rom1v commented Aug 16, 2022

@issess Thank you.

Just for info, does #3448 also fix the problem?

@issess
Copy link
Contributor Author

issess commented Sep 6, 2022

No, It doesn't fixed yet. It can cause conflict my code. So I will modify my commit and apply dev branch again.

@issess
Copy link
Contributor Author

issess commented Sep 6, 2022

I changed base branch from 'master' to 'dev'. please review again.

rom1v pushed a commit that referenced this pull request Sep 9, 2022
@rom1v
Copy link
Collaborator

rom1v commented Sep 9, 2022

Hi,

Sorry for the delay.

I reviewed, and took your branch and applied a few changes:

  • added execReadOutput() to keep execReadLine() as is, that is returning the fist line without any trailing \r and/or \n (this is important for reading and writing settings);
  • removed a few unnecessary capturing blocks from the regex;
  • started the regex with ^ so that it only checks from the beginning of the lines (perfs);
  • extracted the fallback code to a separate function.

Here is a branch: pr3416

One remaining thing to do is parsing flags. For example, on my device, here is an extract of dumpsys display:

    mBaseDisplayInfo=DisplayInfo{"Built-in Screen", displayId 0, FLAG_SECURE, FLAG_SUPPORTS_PROTECTED_BUFFERS, FLAG_TRUSTED, real 1440 x 3120, …

The declared flags must be set in the flags variable.

rom1v pushed a commit that referenced this pull request Sep 9, 2022
@issess issess force-pushed the fix branch 2 times, most recently from 3e6001d to acacf61 Compare September 9, 2022 20:28
@issess
Copy link
Contributor Author

issess commented Sep 9, 2022

Hi,

Sorry for the delay.

I reviewed, and took your branch and applied a few changes:

  • added execReadOutput() to keep execReadLine() as is, that is returning the fist line without any trailing \r and/or \n (this is important for reading and writing settings);
  • removed a few unnecessary capturing blocks from the regex;
  • started the regex with ^ so that it only checks from the beginning of the lines (perfs);
  • extracted the fallback code to a separate function.

Here is a branch: pr3416

One remaining thing to do is parsing flags. For example, on my device, here is an extract of dumpsys display:

    mBaseDisplayInfo=DisplayInfo{"Built-in Screen", displayId 0, FLAG_SECURE, FLAG_SUPPORTS_PROTECTED_BUFFERS, FLAG_TRUSTED, real 1440 x 3120, …

The declared flags must be set in the flags variable.

Great work.
I applied your commit and I implemented stringToFlags method to get display flags.
also, regex ^ doesn't work with multi line text. I tested it. So, I remove the ^
please review again!

@rom1v
Copy link
Collaborator

rom1v commented Sep 9, 2022

Thanks, I will try to review soon.

regex ^ doesn't work with multi line text. I tested it. So, I remove the ^

Even without multi-line, this avoids the regex engine to try to match the regex everywhere in the line (it forces to only match at the beginning of the line, so in theory the perfs are better).

@issess
Copy link
Contributor Author

issess commented Sep 10, 2022

I added Pattern.MULTILINE option. It works.

rom1v pushed a commit that referenced this pull request Sep 10, 2022
rom1v added a commit that referenced this pull request Sep 10, 2022
rom1v pushed a commit that referenced this pull request Sep 10, 2022
rom1v added a commit that referenced this pull request Sep 10, 2022
@rom1v
Copy link
Collaborator

rom1v commented Sep 10, 2022

Thank you.

About the flags capture, since the captured text might include non-flags (fields not beginning with FLAG_), I replaced your String.split() by a separate regex to only capture real flags (and avoid testing reading fields with random names).

I also extracted to a separate function to call the parsing function from unit tests, and I added a unit test: pr3416.2

@issess issess force-pushed the fix branch 2 times, most recently from cf67cce to d97ce7b Compare September 11, 2022 14:36
@issess
Copy link
Contributor Author

issess commented Sep 11, 2022

Thank you.

There is more something to check after Android API 31.

https://cs.android.com/android/platform/superproject/+/master:frameworks/base/core/java/android/view/DisplayInfo.java;drc=db08ffe5f18f9335c07b367b740a94fa97206a80;l=729

In API31 emulator dumpsys display

    mBaseDisplayInfo=DisplayInfo{"Built-in Screen", displayId 0", displayGroupId 0, FLAG_SECURE, FLAG_SUPPORTS_PROTECTED_BUFFERS, FLAG_TRUSTED, real 1080 x 2280, largest app 1080 x 2280, smallest app 1080 x 2280, appVsyncOff 1000000, presDeadline 16666666, mode 1, defaultMode 1, modes [{id=1, width=1080, height=2280, fps=60.000004, alternativeRefreshRates=[]}], hdrCapabilities HdrCapabilities{mSupportedHdrTypes=[], mMaxLuminance=500.0, mMaxAverageLuminance=500.0, mMinLuminance=0.0}, userDisabledHdrTypes [], minimalPostProcessingSupported false, rotation 0, state ON, type INTERNAL, uniqueId "local:4619827259835644672", app 1080 x 2280, density 440 (440.0 x 440.0) dpi, layerStack 0, colorMode 0, supportedColorModes [0], address {port=0, model=0x401cec6a7a2b7b}, deviceProductInfo DeviceProductInfo{name=EMU_display_0, manufacturerPnpId=GGL, productId=1, modelYear=null, manufactureDate=ManufactureDate{week=27, year=2006}, connectionToSinkType=0}, removeMode 0, refreshRateOverride 0.0, brightnessMinimum 0.0, brightnessMaximum 1.0, brightnessDefault 0.39763778}
emulator64_x86_64_arm64:/ $

So, I added test case for API 31, And I change regex from (, FLAG_.*)? to .*?(, FLAG_.*)?

@issess
Copy link
Contributor Author

issess commented Sep 11, 2022

And I added testcase for No FLAG display.

rom1v pushed a commit that referenced this pull request Sep 11, 2022
@rom1v
Copy link
Collaborator

rom1v commented Sep 11, 2022

👍

I rebased on dev, and applied the following minor changes:

  • removed useless ? after .*
  • test == null first to avoid Pattern.compile() if text == null (and to avoid additional indentation)
  • fix typos
diff
diff --git a/server/src/main/java/com/genymobile/scrcpy/wrappers/DisplayManager.java b/server/src/main/java/com/genymobile/scrcpy/wrappers/DisplayManager.java
index 9967c74e..4a2a65be 100644
--- a/server/src/main/java/com/genymobile/scrcpy/wrappers/DisplayManager.java
+++ b/server/src/main/java/com/genymobile/scrcpy/wrappers/DisplayManager.java
@@ -21,8 +21,8 @@ public final class DisplayManager {
     // public to call it from unit tests
     public static DisplayInfo parseDisplayInfo(String dumpsysDisplayOutput, int displayId) {
         Pattern regex = Pattern.compile(
-                "^    mBaseDisplayInfo=DisplayInfo\\{\".*\", displayId " + displayId + ".*?(, FLAG_.*)?, real ([0-9]+) x ([0-9]+).*, rotation ([0-9]+)"
-                        + ".*, layerStack ([0-9]+)",
+                "^    mBaseDisplayInfo=DisplayInfo\\{\".*\", displayId " + displayId + ".*(, FLAG_.*)?, real ([0-9]+) x ([0-9]+).*, rotation " +
+                        "([0-9]+).*, layerStack ([0-9]+)",
                 Pattern.MULTILINE);
         Matcher m = regex.matcher(dumpsysDisplayOutput);
         if (!m.find()) {
@@ -49,17 +49,19 @@ public final class DisplayManager {
 
     private static int parseDisplayFlags(String text) {
         Pattern regex = Pattern.compile("FLAG_[A-Z_]+");
+        if (text == null) {
+            return 0;
+        }
+
         int flags = 0;
-        if (text != null) {
-            Matcher m = regex.matcher(text);
-            while (m.find()) {
-                String flagString = m.group();
-                try {
-                    Field filed = Display.class.getDeclaredField(flagString);
-                    flags |= filed.getInt(null);
-                } catch (NoSuchFieldException | IllegalAccessException e) {
-                    // Silently ignore, some flags reported by "dumpsys display" are @TestApi
-                }
+        Matcher m = regex.matcher(text);
+        while (m.find()) {
+            String flagString = m.group();
+            try {
+                Field filed = Display.class.getDeclaredField(flagString);
+                flags |= filed.getInt(null);
+            } catch (NoSuchFieldException | IllegalAccessException e) {
+                // Silently ignore, some flags reported by "dumpsys display" are @TestApi
             }
         }
         return flags;
diff --git a/server/src/test/java/com/genymobile/scrcpy/CommandParserTest.java b/server/src/test/java/com/genymobile/scrcpy/CommandParserTest.java
index 460835dd..33810265 100644
--- a/server/src/test/java/com/genymobile/scrcpy/CommandParserTest.java
+++ b/server/src/test/java/com/genymobile/scrcpy/CommandParserTest.java
@@ -9,7 +9,7 @@ import org.junit.Test;
 
 public class CommandParserTest {
     @Test
-    public void testParseDisplayInfoFrqomDumpsysDisplay() {
+    public void testParseDisplayInfoFromDumpsysDisplay() {
         /* @formatter:off */
         String partialOutput = "Logical Displays: size=1\n"
                 + "  Display 0:\n"
@@ -51,7 +51,7 @@ public class CommandParserTest {
     }
 
     @Test
-    public void testParseDisplayInfoFrqomDumpsysDisplayAPI31() {
+    public void testParseDisplayInfoFromDumpsysDisplayAPI31() {
         /* @formatter:off */
         String partialOutput = "Logical Displays: size=1\n"
                 + "  Display 0:\n"
@@ -97,7 +97,7 @@ public class CommandParserTest {
     }
 
     @Test
-    public void testParseDisplayInfoFrqomDumpsysDisplayAPI31NoFlags() {
+    public void testParseDisplayInfoFromDumpsysDisplayAPI31NoFlags() {
         /* @formatter:off */
         String partialOutput = "Logical Displays: size=1\n"
                 + "  Display 0:\n"
@@ -136,7 +136,6 @@ public class CommandParserTest {
         Assert.assertEquals(0, displayInfo.getDisplayId());
         Assert.assertEquals(0, displayInfo.getRotation());
         Assert.assertEquals(0, displayInfo.getLayerStack());
-        // FLAG_TRUSTED does not exist in Display (@TestApi), so it won't be reported
         Assert.assertEquals(0, displayInfo.getFlags());
         Assert.assertEquals(1080, displayInfo.getSize().getWidth());
         Assert.assertEquals(2280, displayInfo.getSize().getHeight());

pr3416.3

@issess
Copy link
Contributor Author

issess commented Sep 11, 2022

+1

I rebased on dev, and applied the following minor changes:

  • removed useless ? after .*
  • test == null first to avoid Pattern.compile() if text == null (and to avoid additional indentation)
  • fix typos

diff

diff --git a/server/src/main/java/com/genymobile/scrcpy/wrappers/DisplayManager.java b/server/src/main/java/com/genymobile/scrcpy/wrappers/DisplayManager.java
index 9967c74e..4a2a65be 100644
--- a/server/src/main/java/com/genymobile/scrcpy/wrappers/DisplayManager.java
+++ b/server/src/main/java/com/genymobile/scrcpy/wrappers/DisplayManager.java
@@ -21,8 +21,8 @@ public final class DisplayManager {
     // public to call it from unit tests
     public static DisplayInfo parseDisplayInfo(String dumpsysDisplayOutput, int displayId) {
         Pattern regex = Pattern.compile(
-                "^    mBaseDisplayInfo=DisplayInfo\\{\".*\", displayId " + displayId + ".*?(, FLAG_.*)?, real ([0-9]+) x ([0-9]+).*, rotation ([0-9]+)"
-                        + ".*, layerStack ([0-9]+)",
+                "^    mBaseDisplayInfo=DisplayInfo\\{\".*\", displayId " + displayId + ".*(, FLAG_.*)?, real ([0-9]+) x ([0-9]+).*, rotation " +
+                        "([0-9]+).*, layerStack ([0-9]+)",
                 Pattern.MULTILINE);
         Matcher m = regex.matcher(dumpsysDisplayOutput);
         if (!m.find()) {
@@ -49,17 +49,19 @@ public final class DisplayManager {
 
     private static int parseDisplayFlags(String text) {
         Pattern regex = Pattern.compile("FLAG_[A-Z_]+");
+        if (text == null) {
+            return 0;
+        }
+
         int flags = 0;
-        if (text != null) {
-            Matcher m = regex.matcher(text);
-            while (m.find()) {
-                String flagString = m.group();
-                try {
-                    Field filed = Display.class.getDeclaredField(flagString);
-                    flags |= filed.getInt(null);
-                } catch (NoSuchFieldException | IllegalAccessException e) {
-                    // Silently ignore, some flags reported by "dumpsys display" are @TestApi
-                }
+        Matcher m = regex.matcher(text);
+        while (m.find()) {
+            String flagString = m.group();
+            try {
+                Field filed = Display.class.getDeclaredField(flagString);
+                flags |= filed.getInt(null);
+            } catch (NoSuchFieldException | IllegalAccessException e) {
+                // Silently ignore, some flags reported by "dumpsys display" are @TestApi
             }
         }
         return flags;
diff --git a/server/src/test/java/com/genymobile/scrcpy/CommandParserTest.java b/server/src/test/java/com/genymobile/scrcpy/CommandParserTest.java
index 460835dd..33810265 100644
--- a/server/src/test/java/com/genymobile/scrcpy/CommandParserTest.java
+++ b/server/src/test/java/com/genymobile/scrcpy/CommandParserTest.java
@@ -9,7 +9,7 @@ import org.junit.Test;
 
 public class CommandParserTest {
     @Test
-    public void testParseDisplayInfoFrqomDumpsysDisplay() {
+    public void testParseDisplayInfoFromDumpsysDisplay() {
         /* @formatter:off */
         String partialOutput = "Logical Displays: size=1\n"
                 + "  Display 0:\n"
@@ -51,7 +51,7 @@ public class CommandParserTest {
     }
 
     @Test
-    public void testParseDisplayInfoFrqomDumpsysDisplayAPI31() {
+    public void testParseDisplayInfoFromDumpsysDisplayAPI31() {
         /* @formatter:off */
         String partialOutput = "Logical Displays: size=1\n"
                 + "  Display 0:\n"
@@ -97,7 +97,7 @@ public class CommandParserTest {
     }
 
     @Test
-    public void testParseDisplayInfoFrqomDumpsysDisplayAPI31NoFlags() {
+    public void testParseDisplayInfoFromDumpsysDisplayAPI31NoFlags() {
         /* @formatter:off */
         String partialOutput = "Logical Displays: size=1\n"
                 + "  Display 0:\n"
@@ -136,7 +136,6 @@ public class CommandParserTest {
         Assert.assertEquals(0, displayInfo.getDisplayId());
         Assert.assertEquals(0, displayInfo.getRotation());
         Assert.assertEquals(0, displayInfo.getLayerStack());
-        // FLAG_TRUSTED does not exist in Display (@TestApi), so it won't be reported
         Assert.assertEquals(0, displayInfo.getFlags());
         Assert.assertEquals(1080, displayInfo.getSize().getWidth());
         Assert.assertEquals(2280, displayInfo.getSize().getHeight());

pr3416.3

It's almost done.
but Regex ? is necessary. Testcase will fail.

@rom1v
Copy link
Collaborator

rom1v commented Sep 11, 2022

but Regex ? is necessary. Testcase will fail.

Hmm, indeed, but I don't understand why:

  • .* means "any character, 0 to n times"
  • for me, .*? means "possibly any character, 0 to n times"

Why is ? necessary?

@issess
Copy link
Contributor Author

issess commented Sep 11, 2022

@rom1v
.* means Greedy quantifier. .*? means Lazy quantifier.
see, https://javascript.info/regexp-greedy-and-lazy

@issess
Copy link
Contributor Author

issess commented Sep 11, 2022

I tested regex in https://regex101.com/r/8tpLez/1

rom1v pushed a commit that referenced this pull request Sep 11, 2022
rom1v pushed a commit that referenced this pull request Sep 11, 2022
@issess
Copy link
Contributor Author

issess commented Sep 15, 2022

@rom1v
sync your commit 218a040 and latest dev branch

@rom1v
Copy link
Collaborator

rom1v commented Sep 25, 2022

I pushed pr3416.4 to make checkstyle happy.

However, I noticed a problem when the initial rotation is not the natural orientation (for example by running Firefox while the phone is in landscape mode), the reported rotation is not correct (so mirroring is broken).

The problem is that there are both mBaseDisplayInfo and mOverrideDisplayInfo:

    mBaseDisplayInfo=DisplayInfo{"Built-in Screen", displayId 0, FLAG_SECURE, FLAG_SUPPORTS_PROTECTED_BUFFERS, FLAG_TRUSTED, real 1440 x 3120, largest app 1440 x 31
20, smallest app 1440 x 3120, appVsyncOff 2000000, presDeadline 11111111, mode 2, defaultMode 1, modes [{id=1, width=1440, height=3120, fps=60.0}, {id=2, width=1440
, height=3120, fps=90.0}, {id=3, width=1080, height=2340, fps=90.0}, {id=4, width=1080, height=2340, fps=60.0}], hdrCapabilities HdrCapabilities{mSupportedHdrTypes=
[2, 3, 4], mMaxLuminance=540.0, mMaxAverageLuminance=270.1, mMinLuminance=0.2}, minimalPostProcessingSupported false, rotation 0, state ON, type INTERNAL, uniqueId 
"local:19260779396743297", app 1440 x 3120, density 600 (515.154 x 514.597) dpi, layerStack 0, colorMode 0, supportedColorModes [0, 7, 9], address {port=129, model=
0x446d93aa0a40}, deviceProductInfo DeviceProductInfo{name=, manufacturerPnpId=QCM, productId=1, modelYear=null, manufactureDate=ManufactureDate{week=27, year=2006},
 relativeAddress=null}, removeMode 0}
    mOverrideDisplayInfo=DisplayInfo{"Built-in Screen", displayId 0, FLAG_SECURE, FLAG_SUPPORTS_PROTECTED_BUFFERS, FLAG_TRUSTED, real 3120 x 1440, largest app 3120 x 2983, smallest app 1440 x 1303, appVsyncOff 2000000, presDeadline 11111111, mode 2, defaultMode 1, modes [{id=1, width=1440, height=3120, fps=60.0}, {id=2, width=1440, height=3120, fps=90.0}, {id=3, width=1080, height=2340, fps=90.0}, {id=4, width=1080, height=2340, fps=60.0}], hdrCapabilities HdrCapabilities{mSupportedHdrTypes=[2, 3, 4], mMaxLuminance=540.0, mMaxAverageLuminance=270.1, mMinLuminance=0.2}, minimalPostProcessingSupported false, rotation 3, state ON, type INTERNAL, uniqueId "local:19260779396743297", app 3120 x 1440, density 600 (515.154 x 514.597) dpi, layerStack 0, colorMode 0, supportedColorModes [0, 7, 9], address {port=129, model=0x446d93aa0a40}, deviceProductInfo DeviceProductInfo{name=, manufacturerPnpId=QCM, productId=1, modelYear=null, manufactureDate=ManufactureDate{week=27, year=2006}, relativeAddress=null}, removeMode 0}

I guess that the correct width and height must be read from the mBaseDisplayInfo (when rotation is not applied), but the rotation itself must be read from the mOverrideDisplayInfo). And I'm not even sure that mOverrideDisplayInfo is always present.

rom1v pushed a commit that referenced this pull request Sep 25, 2022
rom1v pushed a commit that referenced this pull request Sep 25, 2022
@rom1v
Copy link
Collaborator

rom1v commented Sep 25, 2022

I guess that the correct width and height must be read from the mBaseDisplayInfo (when rotation is not applied), but the rotation itself must be read from the mOverrideDisplayInfo).

Ah no, everything must be read from mOverrideDisplayInfo. So this patch fixes the problem:

diff --git a/server/src/main/java/com/genymobile/scrcpy/wrappers/DisplayManager.java b/server/src/main/java/com/genymobile/scrcpy/wrappers/DisplayManager.java
index 19568489..bf172126 100644
--- a/server/src/main/java/com/genymobile/scrcpy/wrappers/DisplayManager.java
+++ b/server/src/main/java/com/genymobile/scrcpy/wrappers/DisplayManager.java
@@ -21,8 +21,8 @@ public final class DisplayManager {
     // public to call it from unit tests
     public static DisplayInfo parseDisplayInfo(String dumpsysDisplayOutput, int displayId) {
         Pattern regex = Pattern.compile(
-                "^    mBaseDisplayInfo=DisplayInfo\\{\".*?\", displayId " + displayId + ".*?(, FLAG_.*)?, real ([0-9]+) x ([0-9]+).*?, rotation "
-                        + "([0-9]+).*?, layerStack ([0-9]+)",
+                "^    mOverrideDisplayInfo=DisplayInfo\\{\".*?\", displayId " + displayId + ".*?(, FLAG_.*)?, real ([0-9]+) x ([0-9]+).*?, "
+                        + "rotation ([0-9]+).*?, layerStack ([0-9]+)",
                 Pattern.MULTILINE);
         Matcher m = regex.matcher(dumpsysDisplayOutput);
         if (!m.find()) {

Branch pr3416.5.

(And I added a unit test to check the rotation value and the correct width and height when rotated.)

rom1v pushed a commit that referenced this pull request Sep 25, 2022
@issess
Copy link
Contributor Author

issess commented Sep 26, 2022

@rom1v

You caught the bug well. :) I agree to change from "mBaseDisplayInfo" to "mOverrideDisplayInfo".
And, I sync 949b65d and latest dev branch.

thank you!

@rom1v
Copy link
Collaborator

rom1v commented Sep 27, 2022

Merged into dev 👍

@rom1v rom1v closed this Sep 27, 2022
rom1v pushed a commit that referenced this pull request Nov 12, 2022
rom1v pushed a commit that referenced this pull request Nov 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants